Size Change: +164 B (+0.01%)

Total Size: 1.74 MB

_{compressed-size-action}

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: glendaviesnz <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: scruffian <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: annezazu <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

How does this test for you @jasmussen

This looks great to me. Ideally, as part of showing where styles come from in the future #49278 we can connect the dots to CSS but this feels better than the current situation with the CSS being lost entirely without warning. cc @WordPress/outreach in case anyone has time to test or has additional feedback especially since this has come up a fair amount in feedback from the former FSE Outreach Program.

@scruffian you were the most recent person to touch this file, do you have time to review this change please which keeps custom CSS applied when switching between variations. Background discussion here #56623

Some of my changes have broken things slightly - will try and get this fixed in next day or two

Flaky tests detected in 95e4842.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9296407302
📝 Reported issues:

• [Flaky Test] Redirection after template creation #60678 in /test/e2e/specs/site-editor/site-editor-url-navigation.spec.js
• [Flaky Test] submenu click on the arrow interactions #61884 in /test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js

Thanks @scruffian, are you able to give me some steps to reproduce that issue, for me it clears from variations when clearing styles, but I may be misunderstanding what you are saying:

@jasmussen, @annezazu do you have any thoughts about @scruffian's concerns?

Ben is not wrong, but in this case the exception to the system solves a specific problem, and the merge button is green. I think the main question we need to answer, knowing this, is how we feel about the code quality, the documentation of this exception, and our confidence level of whether this ends up eventually becoming technical debt.

I'd be okay with moving forward, because the omission of this behavior feels unintuitive and non-obvious; CSS currently lives in global styles, but it isn't clear that "styles" are always what people use CSS for, especially as more and more visual properties move to design tools. To that end, the fact that CSS lives in the same scope doesn't necessarily feel fully thought through, if we re-did this I would place it in more of a global scope, one that wasn't even reset when you reset global styles.

But these are not strong opinions and I wouldn't want to ruin Ben's day!

To reproduce the bug:

@jasmussen do you think it also makes sense to make the same exception for other user set styles?

do you think it also makes sense to make the same exception for other user set styles?

Not really, no. I think custom CSS, whether block-local, or global, is the only exception. In fact extracting the font pairings and color palettes to be separate from style variations is one aspect of giving options that aren't as sweeping as style variations are. And yes, a way I think of style variations is to think of them as alternate versions for a theme, changing everything potentially.

We separately have an opportunity to make much more prominent the revision history feature, than it is now. It's an extremely powerful way to rewind accidental changes. Combined with clearer multi entity saving, you might end up in a place where all of these bits are more known.

But yes, custom CSS feels like the exception, and something okay to persist, since it's explicitly typed out.

custom CSS feels like the exception, and something okay to persist, since it's explicitly typed out.

Just looping back and wanted to note that I agree here. I think this change is also more "future proof" in that in the future, there are plans to make the Styles section more obviously globally and refined #53483 which I think would make keeping CSS even more obvious than it is now to do.

@scruffian this should fix the bug you noted if you have time to retest please.

@scruffian if you have a chance to give this another test it would be good to get it into 6.6 RC if you are happy with Joen and Anne's reasoning around the need for this. Let me know if you don't have time and I will ping some others.

LGTM - thanks again 🙇🏻